Skip to content

fix: cherry-pick fixes from main into the 2.23-maintenance branch#2588

Open
tonyandrewmeyer wants to merge 10 commits into
canonical:2.23-maintenancefrom
tonyandrewmeyer:backport-bugfixes-2.23
Open

fix: cherry-pick fixes from main into the 2.23-maintenance branch#2588
tonyandrewmeyer wants to merge 10 commits into
canonical:2.23-maintenancefrom
tonyandrewmeyer:backport-bugfixes-2.23

Conversation

tonyandrewmeyer and others added 10 commits June 26, 2026 13:39
…nonical#2548)

This PR ensures that when closing the Pebble websocket:
* If the socket is already closed, we do nothing.
* If we get an error because it's already closed while closing
(presumably a race), we ignore that.

canonical#2547 has more details on why we have issues in Python 3.13+ when
finalising at the moment.

An alternative would be to change `_wait` to close, but that is riskier,
since it would be a behaviour change and could in theory trigger issues
if someone is doing something odd with a wrapped websocket. Quite
unlikely in our case, but the change in this PR solves the issue and
seems clean enough, and doesn't have the risk.

Fixes canonical#2547

(cherry picked from commit 9d03472)
We don't normally do fixes in Harness, but this will allow charms that
are not yet migrated to Scenario to run with `-Werror` and not get hit
by a resource warning we cause.

`Harness` builds a `framework.Framework` backed by an in-memory
`SQLiteStorage`, but `Harness.cleanup()` never closes it. The sqlite3
connection is only released when the `Harness` is garbage-collected, at
which point its destructor emits `unclosed database in
<sqlite3.Connection ...>`. pytest wraps that as
`PytestUnraisableExceptionWarning` and `-W error` turns it into a test
failure — even for callers who correctly call `harness.cleanup()`.

This is one of the bug classes flagged by a recent `canonical/hyrum`
(super-tox) run across the canonical charm collection: under `-Werror`,
charms whose tests use `ops.testing.Harness` regress even though their
own code is clean.

Changes:

- `Harness.cleanup()`: call `self._framework.close()` after the backend
cleanup so the `SQLiteStorage` connection is closed eagerly. sqlite3's
`close()` is idempotent, so repeated `cleanup()` calls stay safe.
- Add a regression test that asserts the storage's underlying connection
is closed after `harness.cleanup()`.

---------

Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
(cherry picked from commit 54376c1)
`Framework.set_breakpointhook()` has always returned the old hook so the
caller can restore it, but `_Manager._make_framework()` discarded the
return value. In a real Juju hook execution this doesn't matter, since
the process is short-lived, but tests that drive `ops.main()` directly
leave a polluted `sys.breakpointhook` for every subsequent test in the
same process. The leaked hook is a bound `framework.breakpoint` method
whose `_juju_debug_at` came from whichever test set `JUJU_DEBUG_AT`, so
a later test calling `breakpoint()` drops into `pdb` regardless of its
own `PYTHONBREAKPOINT`.

The PR fixes this by saving the return value in `_make_framework()` and
restoring it in `destroy()`. The attribute is a class-level `None` so
that `destroy()` is safe even if `_make_framework()` never ran.

Also fixes a test that relied on the bug.

Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
(cherry picked from commit 9b797aa)
…g Checks (canonical#2567)

When Pebble returns information about a check, it copies the level,
startup, and threshold into the response from the computed plan. If none
of the layers have provided values for these, the plan has defaults
(unset, enabled, and 3, respectively).

When we check a `CheckInfo` for consistency in Scenario (meaning that
the values should be ones Pebble could sensibly return, given the rest
of the state), we need to take that default value into account, as
Pebble would. We don't actually model checks going up and down
currently, so it's really only the consistency that we are concerned
with.

This PR fixes the consistency check so that it will fall back to the
Pebble defaults when needed. It repeats the defaults, because the
existing copy is buried in the mock Pebble client in Harness, and the
source of truth is in the Pebble code.

Fixes canonical#2566

---------

Co-authored-by: Dave Wilding <tech@dpw.me>
Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
(cherry picked from commit a1f3afa)
A few fixes to ensure we do not leak resources (until gc time) in
Scenario:

* In runtime, move the cleanup to a finally block.
* In the Context, instead of using TemporaryDirectory, use mktmpdir
directly, and register a weakref cleanup -- but also let users
explicitly clean up if they prefer (and make the context manager case do
that automatically).
* In state, replace a few opens with read_texts (we could do the open
with a context manager is read but this seemed the more minimal change,
and none of the YAML files should be large enough that there is any
difference).
* In a couple of Scenario's own tests, avoid leaking when using
Container.pull.

After these changes, the tests can be run with -Werror and not have
resource warnings cause failures.

---------

Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
(cherry picked from commit 43b322f)
…nonical#2558)

If the change for an exec doesn't finish before the client-side timeout,
or waiting on it fails in any other way, `ExecProcess._wait()`
propagates the exception without any teardown. The I/O pump threads are
left blocked in `recv()` on the still open websockets, and being
non-daemon threads they then block interpreter shutdown indefinitely,
wedging the Juju hook. See canonical#2556 for the reproduction details (note that
the leak needs the change to outlive the client-side wait, for example
an orphan holding the exec's stdio pipe open; a plain exec timeout
completes the change server-side and already cleans up correctly).

This tears the exec down when the wait fails:

* Shut the websocket connections down at the OS level (`SHUT_RDWR`)
rather than only closing the file descriptors. Closing a socket doesn't
wake other threads blocked in `recv()` on it, so the `finally` approach
suggested in the issue (or joining the threads first, as the success
path does) would hang the hook immediately instead of at exit.
* Treat a connection closed underneath an I/O pump thread
(`WebSocketException` or `OSError`) as end-of-stream rather than letting
the exception escape the thread, which printed noisy
`WebSocketConnectionClosedException` tracebacks via
`threading.excepthook`.
* Create the exec I/O threads as daemon threads, so any teardown path
that's still missed (including never calling `wait()` at all) can't
block interpreter shutdown.
* Close any already-connected websockets when connecting the exec
websockets fails partway through, rather than leaking them (found while
auditing for related leaks).

The behaviour of the success path is unchanged: the threads are still
joined before the websockets are shut down, so output is never
truncated.

Verified against a real Pebble (v1.31.0) on Python 3.10 through 3.14:
before the fix the reproduction from canonical#2556 leaks two `copyfileobj`
threads and hangs at exit; with the fix it raises
`ops.pebble.TimeoutError` as before, leaves no threads behind, and exits
cleanly.

Fixes canonical#2556

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
(cherry picked from commit 00b1e6f)
This PR passes the endpoint name from the Relation down to the actual
hook command, where Juju will use it to validate that it matches the
provided relation ID. Without this, you can trick Ops into a mismatch
between ID and name and get misleading data (although Juju enforces
security, so there is no leakage of data that you couldn't get anyway).

This is mostly just plumbing the extra information through the backend
and into the hookcmds call from the various places we call this command.
There are also a bunch of test updates to include the relation name to
match.

Scenario is updated to reflect the Juju behaviour more closely, and
otherwise expect / accept the name in calls.

I've updated Harness to match, even though we are not typically doing
Harness fixes, because the signature changes and it seems less likely we
would break anyone's existing Harness tests this way, and it's a pretty
straightforward set of changes.

Fixes canonical#2327

---------

Co-authored-by: James Garner <james.garner@canonical.com>
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
(cherry picked from commit 208af9b)
The cherry-picked canonical#2507 ("close SQLite storage in Harness.cleanup()")
makes Harness.cleanup() close the framework's SQLite connection. The
stale postgresql-k8s pin (a6753b27) had a test that called
harness.cleanup() before harness.begin(), which now fails with
"Cannot operate on a closed database".

Bump the pin to c434df80 (the commit main already uses), where the
charm reordered the test to begin() then cleanup().

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Bumping the postgresql-k8s pin to c434df80 (previous commit) fixed the
SQLite-close failure but exposed a second one: that charm now declares
the ops[tracing] extra, so its tests import ops_tracing. The stale
db-charm-tests workflow only patched the local 'ops' dependency and
never installed ops_tracing, giving "ModuleNotFoundError: No module
named 'ops_tracing'".

Backport main's machinery: a _build-wheels reusable workflow that runs
`uv build --all` (producing ops and ops_tracing wheels) and the
tracing-aware install step that adds the ops_tracing wheel when the
charm asks for the extra. The other three charm pins are left at their
known-good older commits since only postgresql-k8s needs tracing.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
canonical#2583 pinned ops-tracing to pydantic>=2,<3 on this branch (main leaves
it unconstrained). That makes ops[tracing] uninstallable for charms
that require pydantic 1 -- notably postgresql-k8s, which pins
pydantic ^1.10 via data_platform_libs -- breaking Data Charm Tests
version resolution. ops-tracing's own code is pydantic-version
agnostic (the vendored libs branch on the major version), so widen the
floor to >=1.10 while keeping the <3 upper bound, matching main's
ability to coexist with pydantic-1 charms.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@tonyandrewmeyer tonyandrewmeyer marked this pull request as ready for review June 30, 2026 01:19

@tromai tromai left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you.

I reviewed the commits after the cherry picked one. I understand why we need those fixes, and the fixes looks good to me.

- name: Update 'ops' dependency in test charm to latest
run: |
ops_wheel="$(ls operator-wheels/ops-[0-9]*.whl)"
ops_tracing_wheel="$(ls operator-wheels/ops_tracing-*.whl)"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder why the pattern for ops_tracing wheel is more relaxed compared to ops_wheel here?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The [0-9] in ops-[0-9]*.whl is defensive: without it, the glob would also match anything starting with ops- (for example a future ops-tools-*.whl). Wheel filenames normalise - to _ in the package portion, so ops_tracing-*.whl already can't be matched by ops-* — there's no similarly-named package to disambiguate against, so the looser pattern is fine.

# extra, so install the matching ops-tracing wheel explicitly when
# the charm asked for it. Check before `poetry remove`, which
# strips the ops line from pyproject.toml.
if grep -qE 'ops[^a-z]+=.*tracing|ops\[[^]]*tracing' pyproject.toml; then

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to double check my understanding, the first sub pattern matches something like

ops = {extras = ["tracing"], version = ">=2.0"}

while the second pattern matches

ops[testing,tracing]

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's right. The first alternative (ops[^a-z]+=.*tracing) catches the Poetry table form like ops = {extras = ["tracing"], ...}, and the second (ops\[[^]]*tracing) catches the PEP 508 extras form like ops[testing,tracing] or ops[tracing].

(I wish all the poetry usage would go away, but some people really like it.)

Comment thread tracing/pyproject.toml
"opentelemetry-sdk~=1.30",
"ops==2.23.3.dev0",
"pydantic>=2,<3",
"pydantic>=1.10,<3",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did we leave this unconstrained on main? I don't mind a bandaid fix in this PR, but this seems like something we should iron out more consistently. Will we be able to declare >=2,<3 in future and rely on hyrum to force the library to work with the charm? Or do we need a more specific solution to relax our constraints in testing?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed this to be constrained on the 2.23 branch because it was needed for some branch resolution that might be important later on (DM if that is too vague).

I would like Pydantic gone in 3.x, so I didn't bother making the change there, but it probably should be too. I think it may also need to be 1.x and above to handle downstream.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, leaving this as-is here for now seems reasonable in that context.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants